-
Notifications
You must be signed in to change notification settings - Fork 12
Various repository fixes #146
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Various repository fixes #146
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks sane to me as is, just nits
crates/composefs/src/util.rs
Outdated
| format!("{}{}", prefix, rand_string) | ||
| } | ||
|
|
||
| pub fn replace_symlinkat( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think unit tests for this would be pretty easy to add too
08611b7 to
362631d
Compare
| let category_fd = self.openat(category, OFlags::RDONLY | OFlags::DIRECTORY)?; | ||
| let category_fd = match filter_errno( | ||
| self.openat(category, OFlags::RDONLY | OFlags::DIRECTORY), | ||
| Errno::NOENT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW I maintain https://docs.rs/cap-std-ext/latest/cap_std_ext/dirext/trait.CapStdExtDirExt.html#tymethod.open_dir_optional
We had some previous debates about using cap-std here. I (obviously) like it a lot.
filter_errno makes sense as is but it'd probably be good to have a higher level openat_optional wrapper per above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just minor changes from me too
crates/composefs/src/repository.rs
Outdated
| Errno::NOENT, | ||
| ) | ||
| .context("Opening {category} dir in repository")? | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is begging for a
let Some(category_fd) = ...? else {
return;
}| } | ||
| } | ||
|
|
||
| Err(Errno::EXIST) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could probably use some context...
362631d to
0f8d2eb
Compare
|
What happened with the commit description? |
|
Uh. There's very clearly some rebase fail here, sorry :( |
0f8d2eb to
fff41b1
Compare
|
This looks good to me, but I added a commit that imho clean up filter_errno(). |
crates/composefs/src/repository.rs
Outdated
| .context("Opening {category} dir in repository")? | ||
| let Some(category_fd) = self | ||
| .openat(category, OFlags::RDONLY | OFlags::DIRECTORY) | ||
| .filter_errno(Errno::NOENT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is deeply cool. Way nicer to use.
Missing DCO, though .... :/
adf4a54 to
454449c
Compare
For example, if there has been no streams created, we don't want to fail with some random ENOENT. Signed-off-by: Alexander Larsson <alexl@redhat.com>
Naming a new version of an image the same as the old version is pretty standard, and this is currently giving EEXIST errors. We fix this by doing an atomic symlink + rename operation to replace the symlink if it doesn't already have the expected value. Drop our existing Repository::ensure_symlink() function and use the new function for that as well. Co-authored-by: Alexander Larsson <alexl@redhat.com> Signed-off-by: Alexander Larsson <alexl@redhat.com> Signed-off-by: Allison Karlitskaya <allison.karlitskaya@redhat.com>
454449c to
7b6075e
Compare
|
(force-pushed to fix the conflict) |
This adds error context when opening images and streams. Without this it is kinda hard to figure out what ENOENT really means. Signed-off-by: Alexander Larsson <alexl@redhat.com>
7b6075e to
968b8b3
Compare
|
@allisonkarlitskaya Pushing this seems to have lost the nicer filter_errno() commit? |
|
I.e. 454449c |
|
The Ubuntu fail is kernel mount regression noise. |
And I missed this, of course. Sorry :( |
|
→ #151 |
These were broken out from: #144